Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Save deleted_by user when deleting products #12431

Closed
wants to merge 2 commits into from
Closed

Save deleted_by user when deleting products #12431

wants to merge 2 commits into from

Conversation

dacook
Copy link
Member

@dacook dacook commented May 2, 2024 •
edited
Loading

Work in progress, but I'd like someone to validate if it's a good idea to proceed

What? Why?

This came up when trying to investigate why products got deleted.
https://openfoodnetwork.slack.com/archives/CDLKH9MM0/p1714272151621489

It seemed like a good idea to log the user that did the delete, so I thought I'd try and prove the concept. I was surprised to find it wasn't already a common pattern, and I think I found out why...

I wasn't sure the best way to set the user, while ensuring it got saved during deletion (and undone if delete fails). Overriding destroy seems like a bad idea but it seems to work too..

Perhaps another way would be to add it to an instance variable from the controller, then copy it during deletion:

# controller
product.to_be_deleted_by = current_user
product.destroy

# model (around_destroy)
destruction do
  transaction do
    # other stuff
    deleted_by = to_be_deleted_by
    yield
  end
end

What should we test?

  • Visit ... page.

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

dacook
dacook commented May 2, 2024
View reviewed changes
db/schema.rb
@@ -10,7 +10,7 @@
#
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: fix up discrepancies here

@dacook dacook self-assigned this May 2, 2024
rioug
rioug reviewed May 7, 2024
View reviewed changes
app/models/spree/product.rb
@@ -260,6 +261,11 @@ def import_date
variants.map(&:import_date).compact.max
end

def destroy(deleted_by: nil)
update_attribute(:deleted_by, deleted_by) if deleted_by.present?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking we shouldn't have a default for deleted_by to enforce recording a user when deleting a product. But I am pretty sure it would blow up in various unexpected places.

@rioug
Copy link
Collaborator

rioug commented May 7, 2024

I think the logic should be in a service, something like ProductDestroyer, then you don't need to override the destroy method.
The service can have all the logic to set/unset deleted_by , it should just be matter of wrapping the logic you used in a transaction block.

@mkllnk
Copy link
Member

mkllnk commented May 7, 2024

I have a different perspective:

  • The database is holding the current state but we want to look at a log of actions.
  • We are adding database constraints that could haunt us later. What if we wanted to delete an admin user who deleted products in the past?

I think that we should think about a logger instead. Spree is logging some stuff in database tables but I think that files are easier to manage. We could just add to the Rails log and make sure it's preserved for longer. Or we could start a separate logger for important actions like this.

The log is usually only written and never read. Only in rare circumstances when something goes wrong, we want to look at the log file.

@dacook
Copy link
Member Author

dacook commented May 7, 2024

I think that we should think about a logger instead.

Thanks, good point, this fits well into the category of logging.
We already log requests like DELETE /products/123, but if it got deleted via an association, it wouldn't be so easy to track (i forget if that's possible). So we could add additional logging for any records (soft-)deleted. This would easily apply to any type of record.
Should it be in the general log file? That's probably fine, because I think deletes don't happen that frequently. Otherwise it might be worth having a separate log file for deletes. It might be helpful logging other actions too, like edits.

The downside of file logs is that we can't easily expose it in the user interface, to let users solve their own questions. Hmm, but it shouldn't be too hard surely.. (eg click a button on the product, Rails greps the log file for that ID and returns the matches).

Ok so I'm sold on file logging.

@dacook dacook closed this May 7, 2024
@dacook dacook mentioned this pull request May 9, 2024
Closed
@dacook
Copy link
Member Author

dacook commented May 9, 2024

I raised #12452 as an alternative

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rioug rioug rioug left review comments

Assignees

@dacook dacook

Labels
None yet
Projects
OFN Delivery board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dacook @rioug @mkllnk